Support for transitive dependency removal#5549
Conversation
mctofu
force-pushed
the
mctofu/remove-dependency-via-flag
branch
from
f26122f to
d2a6ee9
Compare
mctofu
force-pushed
the
mctofu/remove-dependency-via-flag
branch
from
d2a6ee9 to
02a52be
Compare
landongrindheim
left a comment
landongrindheim
left a comment
There was a problem hiding this comment.
I have some questions around how we're using booleans, but I see this doing what we want 😄
| end | ||
|
|
||
| def removed? | ||
| @removed |
There was a problem hiding this comment.
💡 What do you think about this explicitly returning a boolean? I'd expect that based on the ? ending of the method name.
| @removed | |
| !@removed.nil? |
There was a problem hiding this comment.
This should already return a boolean since I've defaulted it to false at https://github.com/dependabot/dependabot-core/pull/5549/files#diff-75ed2e4bde042ddd4c61aeb79efee8d861ecae1351d692fb94607b581d647f05R44
I just wanted the ? on the accessor. Would you do that another way?
There was a problem hiding this comment.
I'd reach for the ? method as well 😄 I'd personally make sure to return true/false because it's a pretty established convention. The other option is !!@removed, but I think Rubocop favors this approach.
| "package_manager" => package_manager, | ||
| "subdependency_metadata" => subdependency_metadata | ||
| "subdependency_metadata" => subdependency_metadata, | ||
| "removed" => removed? ? true : nil |
There was a problem hiding this comment.
Along the same lines, if we were to have #removed? return a boolean, we could just use it here and "removed" would always evaluate to true/false, which feels right to me. If there's an angle I'm not considering here, please let me know 😄
There was a problem hiding this comment.
When false I'm setting it to nil here so that the call to #compact will remove it from the result. That's to avoid altering any behavior until we've made needed adjustments in other systems and turn the feature on.
There was a problem hiding this comment.
👍 My next thought would be to make sure we're handling any use of #fetch to make sure we provide a fallback value in case it's been #compacted away. Glanced through the diff here and I think we're handling the one case that exists 🙂
| return false unless affects_version?(dependency.previous_version) | ||
|
|
||
| # Removing a dependency is a way to fix the vulnerability | ||
| return true if dependency.removed? |
There was a problem hiding this comment.
🎨 This reads so well!
| "Bumps [statesman](https://github.com/gocardless/statesman) "\ | ||
| "and [business](https://github.com/gocardless/business). "\ | ||
| "These dependencies needed to be updated together.\n"\ | ||
| "Removes `statesman`\n"\ |
| removed = update_details.fetch(:removed, false) | ||
| version = update_details.fetch(:version).to_s unless removed |
There was a problem hiding this comment.
🔎 When I read this, I read that version is nil when dependency.removed? is true. Is that always the case? Could the absence of a version suggest removal?
There was a problem hiding this comment.
At this stage of the code, yes. However, it is valid for a Dependency to have just a requirement range (^6.0.0) and no specific version when we are parsing the dependency files and in those cases I didn't want them to be flagged as removed.
| class UpdateChecker < Dependabot::UpdateCheckers::Base | ||
| class VulnerabilityAuditor | ||
| def initialize(dependency_files:, credentials:) | ||
| def initialize(dependency_files:, credentials:, allow_removal: false) |
There was a problem hiding this comment.
How will allow_removal be toggled?
There was a problem hiding this comment.
It's triggered based on the npm_transitive_dependency_removal option being present:
https://github.com/dependabot/dependabot-core/pull/5549/files#diff-90eca29ed297dba81a7ce1bc74ff4d3293e109bfa4e4183f4170d7322a9cbf27R115
On further review, it's going to be easier to merge this first so I can make use of the updated code elsewhere. This change won't cause any downstream issues as long as the feature flag is not enabled. |
3 participants
With security alerts on transitive dependencies we sometimes see that an update to the parent actually ends up removing the vulnerable dependency from the tree. That's a valid way to resolve an alert but removing dependencies is not a feature Dependabot currently supports. This adds limited support for dependency removal (transitive dependencies removed as part of parent upgrade).
The approach is:
UpdateCheckerwill return removed transitive dependencies in the list of dependencies to update but they'll be flagged as removedFileUpdaterwill ignore dependencies flagged as removed since there's nothing to be doneVulnerabilityAuditorbut it's feature flaggedOne complication is that we serialize the dependency information to other internal systems and perform validations in those places as well. Before this can be
mergedenabled I'll need to make corresponding updates to those systems to handle a removed dependency. We'll also need to maintain this feature flag for awhile to prevent errors if this code is used on an older version of GHES which wouldn't have the necessary support for removed dependencies.I'd also considering using a blank target version as a signal of a removed dependency in #5487. However, after I discovered we also use this in other systems I felt a more explicit signal would be easier to understand.
dry-run and PR example